Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

fixing loading from txt file with dask #24

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Conversation

npeschke
Copy link
Contributor

Hi I ran into the issue that loading images from a text file is not possible anymore with dask (brainglobe/cellfinder#184)

This is intended as a temporary fix for this bug until a better solution with imio comes.

@adamltyson
Copy link
Member

Hi @npeschke, thanks for this!

Would you mind adding a test? Just something small to check that a dask array is being generated with this logic? There's test data in this repo that could be used.

moved brain_paths.txt to brain directory
replaced paths with constants
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@npeschke
Copy link
Contributor Author

I added a basic type check for the loading from a txt and an equality check of stack comparing both ways of creating it.
The test takes about a second to run on my machine so I am unsure if it should be marked slow.

@coveralls
Copy link

coveralls commented Sep 15, 2021

Pull Request Test Coverage Report for Build 1238474463

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 86.096%

Totals Coverage Status
Change from base Build 1144092739: 0.03%
Covered Lines: 1096
Relevant Lines: 1273

💛 - Coveralls

@adamltyson adamltyson merged commit e856c2c into brainglobe:main Sep 15, 2021
@adamltyson
Copy link
Member

Looks great, thanks again!

Eventually all of this I/O and dask support should move to imio, so we can support more data formats with dask, across all software. This general issue has been moved to brainglobe/brainglobe-utils#36, if you feel like tackling some more issues @npeschke!

@adamltyson
Copy link
Member

@npeschke is there a need for this to be on PyPI soon, or can it wait for the next release?

@npeschke
Copy link
Contributor Author

It would be nice to have that in PyPI soon, I plan to deploy cellfinder on a HPC cluster and this would make life easier for the admins that install cellfinder. Thanks for asking!

@adamltyson
Copy link
Member

No worries, I'll aim to have it out by the end of the day.

What kind of cluster is it? If you're using SLURM, I can probably help. If not, would you be willing to (very briefly) document the process, to put in the docs and help out others?

@adamltyson
Copy link
Member

cellfinder-core==0.2.8 is out now.

@npeschke
Copy link
Contributor Author

Thank you very much for the new build! Indeed I use as SLURM cluster and I am planning to wrap it into a snakemake workflow.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants